Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix recvmmsg(2) implementation #1341

Merged
merged 1 commit into from
Nov 29, 2020
Merged

Fix recvmmsg(2) implementation #1341

merged 1 commit into from
Nov 29, 2020

Conversation

codeslinger
Copy link
Contributor

There were two problems discovered with the recvmmsg(2) implementation
that this changeset attempts to fix:

  1. As mentioned in Recvmmsg can return fewer than specified number of messages #1325, recvmmsg(2) can return fewer
    messages than requested, and
  2. Passing the return value of recvmmsg(2) as the number of bytes in
    the messages received is incorrect.

This changeset incorporates the proposed fix from /issues/1325,
as well as passing the correct value (mmsghdr.msg_len) for the number
of bytes in a given message.

@codeslinger
Copy link
Contributor Author

codeslinger commented Nov 26, 2020 •
edited
Loading

I don't quite understand why the CI didn't pass on stable. On my Linux machine, the test-aio-drop test passes. It appears to be failing on other open PRs against this repo, as well, without any associated changes to anything that would affect that test. Anybody have any ideas on how to fix this?

@asomers
Copy link
Member

asomers commented Nov 27, 2020

Do you know of any way to force the condition described in #1325, so we can test for it?

@asomers
Copy link
Member

asomers commented Nov 27, 2020

BTW, the failure of test-aio-drop is not related to this PR. I suspect it's due to a bug in glibc. But I've never been able to reproduce it locally, so I'm not sure.

@codeslinger
Copy link
Contributor Author

codeslinger commented Nov 28, 2020 •
edited
Loading

Re: testing: as far as the actual problem that existed prior to this PR, you should be able to:

  1. Create a UDP socket
  2. In one thread, send the socket some number of messages, X
  3. In another thread, call recvmmsg with a number of mmsghdrs > X and a flags value of MSG_DONTWAIT (or MSG_WAITFORONE on Linux)
  4. Ensure that recvmmsg does in fact return with a value of X immediately

I can add a test for that to this PR if that seems acceptable.

@codeslinger
Copy link
Contributor Author

Added a test for short reads when using MSG_DONTWAIT.

@codeslinger
Fix recvmmsg(2) implementation
aef3068 
There were two problems discovered with the `recvmmsg(2)` implementation
that this changeset attempts to fix:

1. As mentioned in /issues/1325, `recvmmsg(2)` can return fewer
   messages than requested, and
2. Passing the return value of `recvmmsg(2)` as the number of bytes in
   the messages received is incorrect.

This changeset incorporates the proposed fix from /issues/1325,
as well as passing the correct value (`mmsghdr.msg_len`) for the number
of bytes in a given message.
@asomers
Copy link
Member

asomers commented Nov 28, 2020

Looks good! Could you also please add a CHANGELOG entry?

@asomers asomers added this to the 0.19.1 milestone Nov 28, 2020
@asomers
Copy link
Member

asomers commented Nov 28, 2020

Actually, in the interest of expediency, I'm going to merge this now and add a CHANGELOG entry later.

asomers
asomers approved these changes Nov 28, 2020
View reviewed changes
Copy link
Member

@asomers asomers left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

bors r+

@bors
Copy link
Contributor

bors bot commented Nov 29, 2020

Build succeeded:

@bors bors bot merged commit c826b52 into nix-rust:master Nov 29, 2020
asomers added a commit to asomers/nix that referenced this pull request Nov 29, 2020
@asomers
[skip ci] update CHANGELOG for PR nix-rust#1341
a588dfd
asomers added a commit that referenced this pull request Nov 29, 2020
@asomers
[skip ci] update CHANGELOG for PR #1341
1d43b2b
asomers added a commit to asomers/nix that referenced this pull request Nov 29, 2020
@asomers
[skip ci] update CHANGELOG for PR nix-rust#1341
95aa8d2
@asomers asomers mentioned this pull request Nov 30, 2020
Closed
@codeslinger codeslinger deleted the fix-recvmmsg branch November 30, 2020 18:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@asomers asomers asomers approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
0.19.1
Development

Successfully merging this pull request may close these issues.
2 participants
@codeslinger @asomers